feat(ebe): mpcv2 signing for all rounds#48
Merged
Conversation
pranavjain97
force-pushed
the
WP-5151-mpc-ecdsa-onprem-ebe
branch
from
d3e467f to
6b810a7
Compare
pranavjain97
requested review from
alextse-bg,
Copilot and
mohammadalfaiyazbitgo
There was a problem hiding this comment.
Pull Request Overview
Adds support for ECDSA MPCv2 signing by defining new request/response fields, implementing a multi‐round signing handler, and covering the flow with integration tests.
- Extend the API spec to include ECDSA MPCv2 fields and response types
- Implement
handleEcdsaMpcV2Signingin the transaction handler with three MPC rounds - Add integration tests and helper utilities for ECDSA MPCv2; update dependencies for keccak hashing
Reviewed Changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/enclavedBitgoExpress/routers/enclavedApiSpec.ts | Add ECDSA MPCv2-specific request fields and response unions |
| src/api/enclaved/handlers/signMpcTransaction.ts | Implement multi-round ECDSA MPCv2 signing logic |
| src/tests/mocks/gpgKeys.ts | Provide mock GPG key pairs for testing |
| src/tests/api/enclaved/signMpcTransaction.test.ts | Add full integration tests for ECDSA MPCv2 rounds |
| src/tests/api/enclaved/ecdsaUtils.ts | Provide helper functions to simulate BitGo MPCv2 signing |
| package.json | Add keccak and its types for hashing in tests |
Comments suppressed due to low confidence (3)
src/api/enclaved/handlers/signMpcTransaction.ts:75
- [nitpick] Rename interface EcdsaSigningParams to EcdsaMpcV2SigningParams to clearly indicate its association with the MPCv2 signing flow.
interface EcdsaSigningParams {
src/api/enclaved/handlers/signMpcTransaction.ts:304
- [nitpick] Correct the pluralization in this error message to 'are supported' to match the list of share types.
`Share type ${shareType} not supported for MPCv2, only MPCv2Round1, MPCv2Round2 and MPCv2Round3 is supported.`,
src/tests/api/enclaved/signMpcTransaction.test.ts:385
- [nitpick] The request property 'pub' is ambiguous; renaming it to 'userGpgPubKey' throughout would improve clarity for API consumers.
};
| // Unified parameters for handleEcdsaSigning - includes all possible fields | ||
| interface EcdsaSigningParams { | ||
| coin: BaseCoin; | ||
| shareType: string; |
There was a problem hiding this comment.
Use the ShareType enum instead of a raw string for the shareType property in EcdsaSigningParams to improve type safety.
Suggested change
| shareType: string; | |
| shareType: ShareType; |
Comment on lines
+345
to
+346
| const [userShare, backupShare, bitgoShare] = await DklsUtils.generateDKGKeyShares(); | ||
| assert(backupShare, 'backupShare is not defined'); |
There was a problem hiding this comment.
[nitpick] The backupShare variable is declared and asserted but not used in the test. Consider removing it or using it to avoid confusion.
Suggested change
| const [userShare, backupShare, bitgoShare] = await DklsUtils.generateDKGKeyShares(); | |
| assert(backupShare, 'backupShare is not defined'); | |
| const [userShare, bitgoShare] = await DklsUtils.generateDKGKeyShares(); |
| @@ -0,0 +1,7 @@ | |||
| declare module 'keccak' { | |||
Contributor
There was a problem hiding this comment.
this is only used for tests so should this be moved to the tests folder?
|
|
||
| switch (shareType.toLowerCase()) { | ||
| case ShareType.MPCv2Round1: { | ||
| const dataKey = await generateDataKey({ keyType: 'RSA-2048', cfg }); |
Contributor
There was a problem hiding this comment.
isn't it AES for aws?
| }); | ||
| return await ecdsaMPCv2Utils.createOfflineRound3Share({ | ||
| txRequest: params.txRequest, | ||
| prv: params.prv, |
Contributor
There was a problem hiding this comment.
is the prv meant to be unencrpyted or encrypted ?
pranavjain97
force-pushed
the
WP-5151-mpc-ecdsa-onprem-ebe
branch
from
e6d7635 to
082cac6
Compare
pranavjain97
force-pushed
the
WP-5151-mpc-ecdsa-onprem-ebe
branch
from
082cac6 to
713f293
Compare
mohammadalfaiyazbitgo
approved these changes
Jul 4, 2025
Comment on lines
+94
to
+96
| encryptedUserGpgPrvKey: t.union([t.undefined, t.string]), | ||
| encryptedRound1Session: t.union([t.undefined, t.string]), | ||
| encryptedRound2Session: t.union([t.undefined, t.string]), |
Contributor
There was a problem hiding this comment.
nit: can use optional(t.string)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket: WP-5151